Bidi ordering #24
Bidi ordering #24
Conversation
benoitkugler
requested review from
andydotxyz and
whereswaldon
as code owners
|
Not sure what is happening with the CI checks |
whereswaldon
left a comment
whereswaldon
left a comment
There was a problem hiding this comment.
I think these changes will work. There may be a "better" way to iterate the runs in-order, but the difference also might not matter for this use-case.
| line = wrapped.Line | ||
|
|
||
| // sort the line by visual order | ||
| sort.Slice(line, func(i, j int) bool { return line[i].VisualIndex < line[j].VisualIndex }) |
There was a problem hiding this comment.
This is fine if we aren't going to do any processing that might rely on the logical ordering, but it is a lossy operation. After this sort, we no longer have easy access to the logical ordering.
In Gio's code, I just traverse the line's VisualOrder array when doing display logic. It's still efficient, but doesn't discard anything.
There was a problem hiding this comment.
Acknowledged, thank you. We could apply your suggestion if required in future work.
Ah, that's the x/tools Go version bug that was resolved by updating the tools package to (details at golang/go#74462) |
Thanks @andydotxyz ! Any hint on how to process to use v0.24.1 ? |
|
Can the go.mod be updated to require that version? Either the indirect section or through a replace. I don't have an example to hand sorry as Fyne uses it directly so our fixes were more straight forward. |
…verall direction of the string
whereswaldon
force-pushed
the
mixed-rtl
branch
from
b84f19f to
dbef80d
Compare
|
Rebasd on top of the CI fix. |
|
@andydotxyz Would you like to have a glance at this PR ? It falls under the bug fix review policy, but since it introduces line wrapping in the render pipeline, I thought you might still want to review it. |
|
Apologies this slipped off the radar - been a tricky couple of weeks. |
andydotxyz
left a comment
andydotxyz
left a comment
There was a problem hiding this comment.
Looks good.
Unless I've misunderstood this isn't adding wrapping at a user level but brings it in as a dependency only.
Future work will find a way to expose that through the API - though we need to think about the "first line" vs "other line" lengths as discussed in previous (non-render?) chat.
Use
LineWrapperto compute bidi visual index; also set the overall direction of the string.Closes #23